Separate top level scope and `globalThis`. by aardvark179 · Pull Request #2161 · mozilla/rhino

aardvark179 · 2025-11-17T16:03:51Z

Another draft PR on the road to separating scopes and scriptable objects (#2163). I've put the rationale for doing this in a separate issue(#2164)

The global scope in EcmaScript is a a little subtle in that it consists of a declaration environment record and an object environment record for the global object (globalThis). The global object contains variable declarations, function declarations, generator function declarations, async function declarations, and async generator function declarations. Everything else (const declarations, let declarations, class declarations, etc.) goes on declaration environment and is not added to globalThis. This change could make that full separation, but currently there is one test in MozillaSuiteTest which depends on a const declaration being added to the global this. I wonder if we should gate that on a language version.

This is working well apart from a couple of edge cases that I think it's worth talking through:

ImporterTopLevel is mostly working, but I don't think I have a great model of how it should work. Currently The behaviour for package lookup is living on the scope half, but I'm wondering if ImporterTopLevel should really put all the behaviour on a custom GlobalThis.
One doc test is using a java method to get the XML implementation associated with the current global scope. Currently that is not part of globalThis, but maybe like java imports it should live on that half.
Module scopes. These have some existing awkward special handling on FunctionObject (which is currently the only way the existing customGlobal test works). The existing ModuleScope model also doesn't match require in node or common-js.

Let me expand on that last point slightly. If I have a file blah.js containing

function thing() { return this; }
exports.thing = thing;
exports.thing2 = thing();

and from node REPL I do

e = require("./blah.js");
thing = e.thing
thing2 = e.thing2
function thing3() { return this; }
thing4 = thing3();

Then we find thing2 === globalThis and thing2 === thing4. require simply introduces a new scope (just like a function does) rather than what we do with a module scope (a new top level scope that happens to have another top level scope as the prototype). What we have doesn't match that, but it also doesn't match the behaviour of import which should have an entirely separate realm (top level scope).

I think I can see two possible ways to resolve this:

If you want custom function provided by a java class on your scope then that java object has to be your globalThis, and the module scope becomes a normal child scope. This would be closest to node as far as I can tell. We might well want to keep the ModuleScope as a special class to mark that we are in a module for relative requires etc.
We keep module scope as a top level scope, but with a globalThis of whatever custom type provided. To avoid mutating the main globalThis when executing code in a module we'd need to ensure all modules execute on child scopes of this main module scope (but those need not themselves be ModuleScopes, and we'd need to find all instanceof ModuleScope checks which currently look at the thisObj and change them to appropriate top scope checks.

I don't know if @youngj is still around, still has their use case, and has an opinion on this. I can't tell how well covered this area really is because our Jacoco reports in rhino only cover the tests in rhino, while the coverage report in tests doesn't seem to be configured to show the core runtime classes.

aardvark179 · 2025-11-18T23:03:37Z

Okay, I've almost got this sorted. There remain a small set of failing tests due to nested scopes and associated values, which think I can tackle. This has required an API change in how we create nested scopes from a common sealed parent. I'm going to try and distill that down to a simple API, maybe something like public Toplevel createIsolate() and public TopLevel.createIsolate(ScriptableObject customGlobal) or something like that. Since this would change our documented API we would need to make a change to the docs as well at some point.

andreabergia · 2025-11-21T08:55:50Z

One concern I have is that it's not great how in so many places you have to do:

        if (scope instanceof TopLevel) {
            scope = ((TopLevel) scope).getGlobalThis();
        }

I am unsure on how we could improve it, but it's really diffused basically everywhere we have Function.call. Feels easy to forget in some place, especially for new code added in the future.

I am not sure why you are doing all the delegation in TopLevel to the globalThis. I have some ideas, but if you could draw a diagram of the scope structure, it would be very helpful for me.

aardvark179 · 2025-11-21T15:53:10Z

@andreabergia I agree that I don't really like the tests for top level that are now scattered through several bits of initialisation code. The original reason to not force the top scope to be a TopLevel was that we had historically allowed custom objects to be used, but I think the separation here allows users to have a custom object as their global this and still force the top level scope to be an actual TopLevel. This would certainly help make behaviour consistent with respect to generator functions and other objects whose prototypes are never created as normal properties of the global scope.

I've prototyped this by doing it in stages

Making the object we create if null is provided as the scope.
Throwing an error if we are passed something that isn't a TopLevel as the scope
Then changing the type.

I'm currently on step 2. and have had make small adjustments to four tests, and have another four which seem to need similar fixes. @gbrail how would you feel about this API change?

aardvark179 · 2025-11-21T17:59:33Z

Okay, I've moved all the initialisation stuff over to requiring a TopLevel, and it wasn't a big change really. I still have one failing test case due to serialisation and class caches. I'll investigate that over the weekend.

There are still couple of explicit checks for TopLevel which I'm also going to try and work through to see which can reasonably be removed without causing too much disruption.

aardvark179 · 2025-11-23T18:57:53Z

Well, that went well. 😆 so it turns out that even if we force the creation of all top level scopes to be actual TopLevels that isn’t enough because of two things

Objects that are created to be scopes but have their parent scope set to null.
Places where we manage to swap the scope and the global this In our code.

The serialisation problem is similar in that the relationship between the top level scope and the global this has been fully set up at the point when something is trying to rely on that.

I’m setting this back to draft until I can track down those problems.

aardvark179 · 2025-11-24T12:12:11Z

Had a burst of inspiration this morning and got this down to about 30 failing test cases. I think all the remaining cases are actual bugs where we ended up with TypeInfoFactory cached against random objects that hadn't had their scopes set up yet. There's a couple of APIs around defining properties which need fixing, it turns out I even did those fixes on my prototype branch for scopes, so I'll try and bring those across to here.

The serialisation problem is occurring because of the way I handled associated values on the top level scope. To enable the creation of isolates I put the values on the global object rather than the scope, and relied on being able to reach that object before it has been associated with top level scope, so the lookup breaks.

I think the thing to do here is to delegate associated values explicitly when creating isolates. I think two top level scopes created from a sealed shared scope should be guaranteed to share the same type cache.

gbrail · 2025-11-25T02:15:23Z

I feel like I'm falling behind here, and apologies -- but it's a testament to the great work you're all doing.

I want to think more carefully about what we're doing to backward compatibility with all this stuff. I've never been super happy about the whole "parent scope" thing, but it's a fundamental part of Rhino and creating a "sealed shared scope" is used by a lot of examples and docs and probably real projects.

Can we document somewhere what we're proposing that will require people to change their current code, and then make a plan to stage it in upcoming releases?

Between this and other things we're working on, as well as the increased velocity of work on the project, it may indeed be time for a 1.9.0 or even a 2.0 with some reasonable changes to backward compatibility in favor of spec compliance.

p-bakker · 2025-11-25T09:35:03Z

Just a possible heads-up when doing stuff in the area of sealed/shared scopes: #1085

aardvark179 · 2025-11-25T11:46:43Z

@gbrail Strongly agree. I think we're approaching the point where we want to do a release prior to any significant API change, and then work to get all those API changes into a 2.0.0 release. I think it might be worth it to create a 2.0 feature branch which we use for this and regularly rebase it on top of master.

I understand why people want to create a sealed parent object as it is a reasonably simple way to avoid repeated initialisation and prevent accidental modification, and we should absolutely ensure there is an API for doing so. I think that can be done, ad we can make it make it easier to use than the current setPrototype/setParentScope dance. That would give people a simple way to do this which we could encourage them to move to, and would hide most of the specifics allowing us to refactor things internally.

aardvark179 · 2025-11-28T18:17:15Z

Okay. Looks like there is a small change required for some Android tests and a little tidy up required. I'll try and do that over the weekend.

rbri · 2025-12-01T18:34:00Z

Puh, this is huge (and i have to attend a workshop the next days). Will try it next week - hope that is ok

And a question - my top level scope is the window (or the globalworker)

Window extends EventTarget
EventTarget extends HtmlUnitScriptable
HtmlUnitScriptable extends ScriptableObject

Do i have to make Window extending TopLevel?

aardvark179 · 2025-12-01T18:52:38Z

Your Window should become the globalThis of a TopLevel, so you should be able to do something like var scope = new TopLevel(new Window) or use the createIsolate API if you want to make a sealed parent.

aardvark179 · 2025-12-13T00:04:25Z

While preparing a refactoring of where we resolve this during function calls I'm seeing a couple of test failures involving isolates and globalThis. I don't think this was a problem introduced by this change, but I am going to try and debug this and work out the best way to resolve it.

aardvark179 · 2025-12-13T12:52:50Z

Okay. The test failures I saw appear to be long term bugs in our code base which I've fixed as part of my refactoring and nothing introduced by this change.

aardvark179 · 2025-12-17T11:59:58Z

So, reviewing my own change…

I don't like the way we build isolates with a custom Java class, but I didn't like the old way either. They both involved messing with the prototype chain making that custom class unusable as a normal object (or at least really weird since it exposed all of global this in its prototype chain) and it required special handling in FunctionObject.

I think I have a better model of how to do this, which works by using bound functions and avoids any messing with prototype chains.

aardvark179 · 2025-12-19T12:44:06Z

After some experimentation I've gone for cleaning up ModuleScope, and have got a fairly clean special case in FunctionObject on my follow-up clean up of this handling. I still don't like the way we handle isolates with custom classes in RequireTest.customGlobal, but we appear to have specific tests that we aren't implementing this using bound functions or similar tricks, so I guess the people who use this fearure actually depend on that behaviour.

rbri · 2026-03-15T16:41:10Z

Still think we have a issue with NativeConsole.init(Scriptable scope, boolean sealed, ConsolePrinter printer)

Using the provided scope for all the calls looks good to me, but the last line

ScriptableObject.defineProperty(scope, "console", obj, ScriptableObject.DONTENUM);

should define the property on the globalThis. Therefore i expect to see also the destinationObject for the new property in the method parameter list.

rbri · 2026-03-15T16:51:05Z

During the last weeks i tried to apply this change to HtmlUnit. It was a hard fight, because there was no clear separation between the globalThis and the TopScope. I think i'm nearly done and fine with this - means: happy to see this merged!

But we have to have in mind, how big this change might be for other applications using Rhino.

Maybe it will be helpfuly to have a clear design sample document including some pictures and sample code that shows how to use Rhino as scripting language for an application.

Including

the TopLevel / Scope idea
the globalThis object
the parentScope hirachy (including the parentScope setup for the globalThis object)
the helper methods to walk to the TopLevel
how to add you own implementation of
- objects/prototypes
- object properties
- object functions
if a function creates a new object - how to setup
- the scope
- the prototype (this includes a description how to find the prototype for all user defined objects)

And maybe more (hope the AI can help us a bit to make a shiny document here). Maybe we can start small and improve this step by step

What do you think?

aardvark179 · 2026-03-16T00:59:17Z

So, any property defined on TopLevel is simply defined on the associated globalThis object. That should not remain true in the long term (because consts, classes, and some other things shouldn't end up there, but for now it's true, and is why it does not matter that scope is being passed in that property definition call rather than globalThis. Clearly I need to find a good way to communicate that.

@rbri would you be okay with the architectural docs and examples being a follow up PR so we can merge this and start to work through the PR stack? I think I agree with your list, but I think it's also going to need a little bit of a primer on what realms are, what intrinsic are, and a couple of other spec related things.

P.s. Sorry, that came over grumpier than I intended. My jet lagged brain clearly isn't doing tone well today.

means: happy to see this merged!

🎉 Ship it @gbrail! 😄

aardvark179 · 2026-03-16T01:04:39Z

Oh wait, I need to rebase this before we can merge it.

rbri · 2026-03-16T07:26:47Z

@rbri would you be okay with the architectural docs and examples being a follow up PR so we can merge this and start to work through the PR stack? I think I agree with your list, but I think it's also going to need a little bit of a primer on what realms are, what intrinsic are, and a couple of other spec related things.

That was exactly the reason for my last post, i think we have to go this way and i no longer like to be something like the blocker for this and all the other cool things on top.

rbri · 2026-03-16T07:29:04Z

P.s. Sorry, that came over grumpier than I intended. My jet lagged brain clearly isn't doing tone well today.

@aardvark179 at first, my english is not good enought to even notice this ;-) And i fear sometimes my writings have the same 'problem'

And second: we are 'fighting' for the best solution - thats at least for me the fun part of all this

gbrail

Let's get this thing moving! It's an important next step in our evolution, and since it will affect every Rhino embedder it's time to rip the band aid off and work on this.

With that said, I have a few small suggestions but if other's are generally OK I think that we should move forward soon. Thanks!

gbrail · 2026-03-18T02:57:36Z

+    }
+
+    @Override
+    public Object get(String name, Scriptable start) {


What breaks if this class overrides get, put, etc, but does not override getIds? Are there cases in which some code will need to enumerate the properties of the global object?

Also is there a reason to override get(int) and put(int)?

There's enough annoying edge cases with things like __parent__ that I thought it best to override everything.

Sorry I was going to fast - why NOT override those two as well? Same with getIds?

gbrail · 2026-03-18T03:08:49Z

    public FunctionObject(String name, Member methodOrConstructor, Scriptable scope) {
        if (methodOrConstructor instanceof Constructor) {
-            member = new MemberBox((Constructor<?>) methodOrConstructor);
+            member = new MemberBox(getDeclarationScope(), (Constructor<?>) methodOrConstructor);


Have not verified this myself but by AI friend tells me that at this point in the constructor, getDeclarationScope() will always return null. That looks accurate to me. Is that going to cause a problem later with the Java embedding?

Good catch. This was an artefact of re-ordering a bunch of commits a while ago.

gbrail · 2026-03-18T03:14:38Z


        Function f = getCallbackArg(cx, callbackArg);
-        Scriptable parent = ScriptableObject.getTopLevelScope(f);
+        TopLevel parent = ScriptableObject.getTopLevelScope(f);


Just a minor thing but in my new Java code I have been getting more judicious about using "var" in cases like this and if we had done so before then perhaps this would not have been necessary to make this particular change.

Yes, I've been using var a lot and it would have reduced a bunch of these changes, but I've had enough push back on changing explicit types to var that I'm wary of doing it as part of a complex behavioural change.

gbrail · 2026-03-20T15:58:07Z

@rbri this is the first of many that will now start to cause backward compatibility to break, but we've all been talking about this for long enough and we agree it's time. Are you OK? I want to start moving faster on Duncan's big rewrite!

rbri · 2026-03-20T17:12:31Z

Are you OK? I want to start moving faster on Duncan's big rewrite!

@gbrail all fine for me - let'S move on

gbrail · 2026-03-20T23:07:34Z

OK -- let's do it and move to the next one. Thanks, all!

aardvark179 · 2026-03-21T00:34:04Z

Cool. Next in the queue should be the changes to function this resolution ( #2328 ), I'll get that prepped when I'm get back home tomorrow.

Oh, wait, I have WiFi. I've done the rebase and made that PR ready for review.

andreabergia · 2026-04-22T13:49:54Z

+
+        var obj = (Scriptable) scope.get("Object", scope);
+        var objProto = (Scriptable) obj.get("prototype", obj);
+        globalThis.setPrototype(objProto);


Why is this wiring done here, and not somewhere else like in the initialization of TopLevel or at the end of initStandardObject?

andreabergia · 2026-04-22T14:19:09Z

+        globalThis = customGlobal;
+    }
+
+    public static TopLevel createIsolate(TopLevel parent) {


Why the name? I assume it has something to do with v8 isolates (which I don't know well), but since it actually creates a relationship with the existing toplevel via the prototype and builtins, it's a bit confusing.

aardvark179 force-pushed the aardvark179-separate-global-this branch from 2fecd46 to 10c5f88 Compare November 19, 2025 15:28

aardvark179 marked this pull request as ready for review November 19, 2025 15:29

aardvark179 marked this pull request as draft November 23, 2025 18:58

aardvark179 force-pushed the aardvark179-separate-global-this branch from 890f88d to 49f3a22 Compare November 28, 2025 16:48

aardvark179 force-pushed the aardvark179-separate-global-this branch 3 times, most recently from c161013 to 7ee1037 Compare December 1, 2025 18:30

aardvark179 marked this pull request as ready for review December 1, 2025 18:31

aardvark179 force-pushed the aardvark179-separate-global-this branch from 7ee1037 to 82bcce9 Compare December 4, 2025 17:06

aardvark179 force-pushed the aardvark179-separate-global-this branch from 82bcce9 to 2d051f2 Compare December 12, 2025 10:36

aardvark179 force-pushed the aardvark179-separate-global-this branch from 2d051f2 to 195618b Compare December 16, 2025 10:38

aardvark179 force-pushed the aardvark179-separate-global-this branch from 195618b to b0cbdc0 Compare December 19, 2025 10:42

aardvark179 force-pushed the aardvark179-separate-global-this branch 2 times, most recently from 03d7ed4 to 3b0db1c Compare December 21, 2025 17:24

aardvark179 force-pushed the aardvark179-separate-global-this branch from 63e71ec to 974f703 Compare March 16, 2026 02:25

gbrail reviewed Mar 18, 2026

View reviewed changes

aardvark179 added 14 commits March 17, 2026 22:52

Separate globalThis and TopLevel.

234edce

Move java importing to globalThis and refactor module scopes.

923639b

Introduce createIsolate to make use of shared sealed scopes easier.

95dd979

Refactor to require TopLevel

a7c0521

Dealing with property definition stuff.

b79475a

Fix up dynamic scope test.

cf32736

Fix serialisation issues.

b0df6c9

Require fixup.

836e4dd

XML doctest fixup.

8138693

Fix Android ITs.

cb8105d

Clean up ModuleScope.

cff270a

Alter isolate API to handle edge cases properly.

1ce6d52

Fixup after rebase.

74840c1

Small tidy up as suggested in review.

c6cbf52

aardvark179 force-pushed the aardvark179-separate-global-this branch from 974f703 to c6cbf52 Compare March 18, 2026 05:57

gbrail merged commit 74d9c79 into mozilla:master Mar 20, 2026
11 checks passed

aardvark179 deleted the aardvark179-separate-global-this branch March 27, 2026 10:53

andreabergia reviewed Apr 30, 2026

View reviewed changes

Uh oh!

Conversation

aardvark179 commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aardvark179 commented Nov 18, 2025

Uh oh!

andreabergia commented Nov 21, 2025

Uh oh!

aardvark179 commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aardvark179 commented Nov 21, 2025

Uh oh!

aardvark179 commented Nov 23, 2025

Uh oh!

aardvark179 commented Nov 24, 2025

Uh oh!

gbrail commented Nov 25, 2025

Uh oh!

p-bakker commented Nov 25, 2025

Uh oh!

aardvark179 commented Nov 25, 2025

Uh oh!

aardvark179 commented Nov 28, 2025

Uh oh!

rbri commented Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aardvark179 commented Dec 1, 2025

Uh oh!

aardvark179 commented Dec 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aardvark179 commented Dec 13, 2025

Uh oh!

aardvark179 commented Dec 17, 2025

Uh oh!

aardvark179 commented Dec 19, 2025

Uh oh!

rbri commented Mar 15, 2026

Uh oh!

rbri commented Mar 15, 2026

Uh oh!

aardvark179 commented Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aardvark179 commented Mar 16, 2026

Uh oh!

rbri commented Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rbri commented Mar 16, 2026

Uh oh!

gbrail left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gbrail commented Mar 20, 2026

Uh oh!

rbri commented Mar 20, 2026

Uh oh!

gbrail commented Mar 20, 2026

Uh oh!

Uh oh!

aardvark179 commented Mar 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

aardvark179 commented Nov 17, 2025 •

edited

Loading

aardvark179 commented Nov 21, 2025 •

edited

Loading

rbri commented Dec 1, 2025 •

edited

Loading

aardvark179 commented Dec 13, 2025 •

edited

Loading

aardvark179 commented Mar 16, 2026 •

edited

Loading

rbri commented Mar 16, 2026 •

edited

Loading

aardvark179 commented Mar 21, 2026 •

edited

Loading